Replays - Install missing map #13233

rob-v · 2017-05-03T19:11:14Z

Replay's Map preview unification with Lobby map preview - Install map, map author, map title tooltip, ...

pchote

The overall approach here looks good, and naturally extends towards our wishlist goal of showing players and spawns in the server list map previews. I haven't looked at the replay part of this in any detail yet, but here's some issues with the map preview logic to start with.

Please also split this into separate commits for refactoring (changing LobbyMapPreviewLogic to work from additional places) vs new features (making the ReplayBrowserLogic use it). This makes it much easier to understand changes when looking back through the git history.

pchote · 2017-05-06T08:32:27Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyMapPreviewLogic.cs

-						m => WidgetUtils.TruncateText("Created by {0}".F(lobby.Map.Author), authorLabel.Bounds.Width, font));
-					authorLabel.GetText = () => author.Update(lobby.Map);
-				}
+				available.IsVisible = () => getMap().Status == MapStatus.Available && (!getMap().RulesLoaded || !getMap().InvalidCustomRules);


Our map handling code and parts of our UI are multithreaded, so seeing lines like this makes me uneasy.

We knew from the design of LobbyLogic that lobby.Map would not change between evaluating e.g. lobby.Map.Status andlobby.Map.RulesLoaded, but we don't have that guarantee for arbitrary getMaps.

Please change these to query getMap() once per evaluation, e.g. here:

available.IsVisible = () => { var map = getMap(); return map.Status == MapStatus.Available && (!map.RulesLoaded || !map.InvalidCustomRules); };

The natural question that follows this is whether we can cache getMap across the different widget bindings. The answer is probably, but it is probably not worth it. You could in principle override the ChromeLogic.Tick and then set a field in the class that is used in all the bindings, but I expect that this would introduce a noticable latency, and possibly other side effects.

So long as the map usage is consistent within individual bindings, it's generally not a big deal if it changes between evaluating different widgets. The worst case scenario is that e.g. the title and author labels are out of sync for 1/60th of a second until the next frame is rendered.

pchote · 2017-05-06T08:50:32Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyMapPreviewLogic.cs


 				var install = download.GetOrNull<ButtonWidget>("MAP_INSTALL");
 				if (install != null)
 				{
-					install.OnClick = () => lobby.Map.Install(mapRepository, () =>
+					install.OnClick = () => getMap().Install(mapRepository, () =>


This is a good example of the getMap() danger: it generally takes between half and a few seconds to install a map, and its quite possible for the host to change map between getMap().Install and getMap().PreloadRules running.

The fix again is to evaluate it once to be certain that it can't change under you:

install.OnClick = () => { var map = getMap(); map.Install(mapRepository, () => { map.PreloadRules(); if (orderManager != null) Game.RunAfterTick(() => orderManager.IssueOrder(Order.Command("state {0}".F(Session.ClientState.NotReady)))); } };

pchote · 2017-05-06T08:51:11Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyMapPreviewLogic.cs

@@ -22,100 +24,39 @@ public class LobbyMapPreviewLogic : ChromeLogic
 		int blinkTick;

 		[ObjectCreator.UseCtor]
-		internal LobbyMapPreviewLogic(Widget widget, ModData modData, OrderManager orderManager, LobbyLogic lobby)
+		internal LobbyMapPreviewLogic(Widget widget, ModData modData, OrderManager orderManager, Func<MapPreview> getMap,


This is no longer Lobby specific, so the class and file should be renamed.

pchote · 2017-05-06T08:51:45Z

OpenRA.Mods.Common/Widgets/Logic/ReplayBrowserLogic.cs

@@ -27,24 +27,28 @@ public class ReplayBrowserLogic : ChromeLogic
 	{
 		static Filter filter = new Filter();

+		public MapPreview Map { get; private set; }


This shouldn't need to be public?

pchote · 2017-05-06T08:55:37Z

OpenRA.Mods.Common/Widgets/Logic/ReplayBrowserLogic.cs


 			if (replay == null)
 				return;

 			try
 			{
+				if (Map.Status != MapStatus.Available)


Style nit: don't leave hanging blocks. The hanging else-if is ambiguous between the inner if and the outer if (with an indentation error), so this needs braces to make it explicit.

rob-v · 2017-05-06T13:55:14Z

Updated.

Code updated.

chrisforbes

Minor nits, but I like it.

chrisforbes · 2017-05-13T18:21:52Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/LobbyLogic.cs


+		MapPreview Map { get; set; }


Why is this a property at all, if you're making both accessors private?

LobbyLogic.Map was used in LobbyUtils so public get. I pass now Map instead of LobbyLogic so public was removed. I'll change it to field.

chrisforbes · 2017-05-13T18:22:38Z

OpenRA.Mods.Common/Widgets/Logic/ReplayBrowserLogic.cs


-		Dictionary<CPos, SpawnOccupant> selectedSpawns;
+		MapPreview Map { get; set; }


Ditto, may as well just be a field here.

rob-v · 2017-05-13T19:04:34Z

Updated - property Map replaced by field.

pchote

Looks good, thanks for taking this feature on. 👍 after a few small tweaks:

pchote · 2017-05-14T11:16:38Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/MapPreviewLogic.cs

+					var map = getMap();
+					return map.Status == MapStatus.Available && (!map.RulesLoaded || !map.InvalidCustomRules);
+				};
+				SetupWidgets(available, getMap, onMouseDown, getSpawnOccupants);


Style nit: newline between closing braces and the next statement.

pchote · 2017-05-14T11:16:48Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/MapPreviewLogic.cs

+					var map = getMap();
+					return map.Status == MapStatus.Available && map.InvalidCustomRules;
+				};
+				SetupWidgets(invalid, getMap, onMouseDown, getSpawnOccupants);


pchote · 2017-05-14T11:16:57Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/MapPreviewLogic.cs

+					var map = getMap();
+					return map.Status != MapStatus.Available && map.Status != MapStatus.DownloadAvailable;
+				};
+				SetupWidgets(progress, getMap, onMouseDown, getSpawnOccupants);


pchote · 2017-05-14T11:17:49Z

OpenRA.Mods.Common/Widgets/Logic/Lobby/MapPreviewLogic.cs

+					statusSearching.IsVisible = () => getMap().Status == MapStatus.Searching;
+
+				var statusUnavailable = progress.GetOrNull("MAP_STATUS_UNAVAILABLE");
+				if (statusUnavailable != null)


Style nit: put braces around hanging blocks.

pchote · 2017-05-14T11:23:01Z

OpenRA.Mods.Common/Widgets/Logic/ReplayBrowserLogic.cs

+				{ "getMap", (Func<MapPreview>)(() => Map) },
+				{ "onMouseDown",  (Action<MapPreviewWidget, MapPreview, MouseInput>)((preview, map, mi) => { }) },
+				{ "getSpawnOccupants", (Func<MapPreview, Dictionary<CPos, SpawnOccupant>>)(map => LobbyUtils.GetSpawnOccupants(selectedReplay.GameInfo.Players, map)) },
+			});

 			panel.Get<LabelWidget>("DURATION").GetText = () => WidgetUtils.FormatTimeSeconds((int)selectedReplay.GameInfo.Duration.TotalSeconds);


Can you please change this to the following while you're here?

var replayDuration = new CachedTransform<ReplayMetadata, string>(r => "Duration: {0}".F(WidgetUtils.FormatTimeSeconds((int)selectedReplay.GameInfo.Duration.TotalSeconds))); panel.Get<LabelWidget>("DURATION").GetText = () => replayDuration.Update(selectedReplay);

This makes the label look less disconnected when the install map button is showing, plus a small perf win.

pchote · 2017-05-14T11:25:43Z

OpenRA.Mods.Common/Widgets/Logic/ReplayBrowserLogic.cs

-			selectedSpawns = (selectedReplay != null)
-				? LobbyUtils.GetSpawnOccupants(selectedReplay.GameInfo.Players, selectedReplay.GameInfo.MapPreview)
-				: new Dictionary<CPos, SpawnOccupant>();
+			map = (selectedReplay != null) ? selectedReplay.GameInfo.MapPreview : MapCache.UnknownMap;


Style nit: unnecessary parenthesis around selectedReplay != null.

rob-v · 2017-05-14T12:33:14Z

Updated.

pchote · 2017-05-14T20:07:07Z

https://github.com/OpenRA/OpenRA/wiki/Changelog-(bleed)/_compare/210ca7802efda00c007f8bcd6bb8214897054582

LipkeGu · 2017-05-17T03:36:04Z

Can anyone agree that the PR is getting compiled .... i get errors like "local variable map is already define and gets overwritten"

reaperrr · 2017-05-17T16:20:10Z

i get errors like "local variable map is already define and gets overwritten"

Same here, the last commit is to blame. Needs to be reverted or fixed.

rob-v · 2017-05-17T16:27:49Z

The PR before merge was built succesfully (also locally). There is simple fix to avoid this 'warning as error'. Could you please review quickly #13071? It by chance removes the local 'map' argument so solves this issue.

reaperrr · 2017-05-17T16:37:22Z

#13071 reduces the errors, but lines 387 and 401 in LobbyLogic still have that issue there.

pchote · 2017-05-17T16:39:43Z

This should be fixed as its own PR, separate to adding any new features.

rob-v · 2017-05-17T16:45:00Z

#13071 Updated - map parameter renamed, compiled now with the same MSBuild, so should be fine now.

rob-v · 2017-05-17T17:02:28Z

Fixed by #13313.

rob-v mentioned this pull request May 3, 2017

[Replay] Missing requiered maps should be auto downloaded... or embedded #11149

Closed

rob-v closed this May 4, 2017

rob-v reopened this May 4, 2017

pchote previously requested changes May 6, 2017

View reviewed changes

pchote added the PR: Changes requested label May 6, 2017

pchote removed the PR: Changes requested label May 13, 2017

pchote mentioned this pull request May 13, 2017

Display team arrangement in Lobby and Replay #13071

Closed

chrisforbes approved these changes May 13, 2017

View reviewed changes

chrisforbes added the PR: Needs +2 label May 13, 2017

pchote approved these changes May 14, 2017

View reviewed changes

pchote added the PR: Fixup requested label May 14, 2017

rob-v added 3 commits May 14, 2017 14:17

(Lobby)MapPreviewLogic refactoring

8d92136

Replays with MapPreview (like in Lobby)

0f8bb05

LobbyLogic, ReplayBrowserLogic Map property changed to map field

74bd56b

abcdefg30 removed the PR: Fixup requested label May 14, 2017

pchote merged commit 127ef8b into OpenRA:bleed May 14, 2017

rob-v deleted the ReplayInstallMap branch May 18, 2017 07:48

pchote mentioned this pull request Jun 26, 2017

System.NullReferenceException on desert-shellmap #13554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replays - Install missing map #13233

Replays - Install missing map #13233

rob-v commented May 3, 2017 •

edited

pchote left a comment

pchote May 6, 2017

pchote May 6, 2017

pchote May 6, 2017

pchote May 6, 2017

pchote May 6, 2017

pchote May 6, 2017

rob-v commented May 6, 2017

chrisforbes left a comment

chrisforbes May 13, 2017

rob-v May 13, 2017

chrisforbes May 13, 2017

rob-v commented May 13, 2017

pchote left a comment

pchote May 14, 2017 •

edited

pchote May 14, 2017

pchote May 14, 2017

pchote May 14, 2017

pchote May 14, 2017

pchote May 14, 2017

rob-v commented May 14, 2017

pchote commented May 14, 2017

LipkeGu commented May 17, 2017

reaperrr commented May 17, 2017

rob-v commented May 17, 2017 •

edited

reaperrr commented May 17, 2017

pchote commented May 17, 2017

rob-v commented May 17, 2017

rob-v commented May 17, 2017


		Dictionary<CPos, SpawnOccupant> selectedSpawns;
		MapPreview Map { get; set; }

Replays - Install missing map #13233

Replays - Install missing map #13233

Conversation

rob-v commented May 3, 2017 • edited

pchote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob-v commented May 6, 2017

chrisforbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob-v commented May 13, 2017

pchote left a comment

Choose a reason for hiding this comment

pchote May 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob-v commented May 14, 2017

pchote commented May 14, 2017

LipkeGu commented May 17, 2017

reaperrr commented May 17, 2017

rob-v commented May 17, 2017 • edited

reaperrr commented May 17, 2017

pchote commented May 17, 2017

rob-v commented May 17, 2017

rob-v commented May 17, 2017

rob-v commented May 3, 2017 •

edited

pchote May 14, 2017 •

edited

rob-v commented May 17, 2017 •

edited